src: print exceptions from PromiseRejectCallback by addaleax · Pull Request #29513 · nodejs/node

addaleax · 2019-09-09T21:45:19Z

Previously, leaving the exception lying around would leave the JS
engine in an invalid state, as it was not expecting exceptions to
be thrown from the C++ PromiseRejectCallback, and lead to hard
crashes under some conditions (e.g. with coverage enabled).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Previously, leaving the exception lying around would leave the JS engine in an invalid state, as it was not expecting exceptions to be thrown from the C++ `PromiseRejectCallback`, and lead to hard crashes under some conditions (e.g. with coverage enabled).

benjamingr · 2019-09-09T22:33:39Z

I am confused, when would our PromiseRejectCallback ever throw? I am probably tired but:

we export setPromiseRejectCallback
We call it with handlePromiseRejections
promiseRejectHandler adds the rejection to the weakmap here and calls setHasRejectionToWarn which sets kHasRejectionToWarn to true on the tickinfo.
That gets processed in processTicksAndRejections which is always at a later point.

I am not sure how/why handlePromiseRejections would throw. I might be completely misunderstanding things :]

Actual technical changes LGTM, I am not ✅ing simply because I did not checkout the code and reproduce what this fixed because I am not sure how to make it throw.

addaleax · 2019-09-09T22:44:01Z

@benjamingr If you look at test/parallel/test-ttywrap-stack.js, you’ll see that it generates unhandled rejections near the stack limit. In that case, the PromiseRejectCallback may also throw an exception, namely a stack depth exceeded exception, when it tries to run JS code. Currently, when running the test with NODE_V8_COVERAGE set, that makes the test crash hard, because the exception isn’t used anywhere and the inspector’s console.log() code in V8 doesn’t expect to be run while there is a scheduled/pending exception.

(I’m not sure but it’s probably also possible to make the callback crash by overriding internals enough, e.g. WeakMap.prototype.set.)

benjamingr

Thanks for explaining how to test this Anna :]

Changes LGTM

nodejs-github-bot · 2019-09-12T07:43:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25437/

Trott · 2019-09-12T07:44:46Z

Some minor JS lint errors to fix:

/home/travis/build/nodejs/node/test/parallel/test-promise-reject-callback-exception.js
   2:7  error  'common' is assigned a value but never used   no-unused-vars
  25:1  error  Expected indentation of 9 spaces but found 6  indent
  28:1  error  Expected indentation of 9 spaces but found 6  indent

addaleax · 2019-09-12T20:04:52Z

Thanks, CI should be fixed now – also updated another test to account for the changes here.

nodejs-github-bot · 2019-09-12T20:05:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25456/

Trott · 2019-09-12T22:23:49Z

Documenting here so people don't do futile "Resume Build" runs: At least some CI failures appear to be relevant edge cases. One example:

https://ci.nodejs.org/job/node-test-commit-linux-containered/14774/nodes=ubuntu1604_sharedlibs_withoutintl_x64/console

test-digitalocean-ubuntu1604_sharedlibs_container-x64-10

13:09:28 not ok 1630 parallel/test-promise-reject-callback-exception
13:09:28   ---
13:09:28   duration_ms: 0.266
13:09:28   severity: fail
13:09:28   exitcode: 1
13:09:28   stack: |-
13:09:28     assert.js:373
13:09:28         throw err;
13:09:28         ^
13:09:28     
13:09:28     AssertionError [ERR_ASSERTION]: stdout: <>
13:09:28         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-promise-reject-callback-exception.js:23:3)
13:09:28         at Module._compile (internal/modules/cjs/loader.js:936:30)
13:09:28         at Object.Module._extensions..js (internal/modules/cjs/loader.js:947:10)
13:09:28         at Module.load (internal/modules/cjs/loader.js:789:32)
13:09:28         at Function.Module._load (internal/modules/cjs/loader.js:702:12)
13:09:28         at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
13:09:28         at internal/main/run_main_module.js:17:11 {
13:09:28       generatedMessage: false,
13:09:28       code: 'ERR_ASSERTION',
13:09:28       actual: false,
13:09:28       expected: true,
13:09:28       operator: '=='
13:09:28     }
13:09:28   ...

addaleax · 2019-09-13T11:52:26Z

@Trott Thanks, I’ve opened #29542 to track that, and added a workaround to the test for now.

nodejs-github-bot · 2019-09-13T11:53:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25471/

Previously, leaving the exception lying around would leave the JS engine in an invalid state, as it was not expecting exceptions to be thrown from the C++ `PromiseRejectCallback`, and lead to hard crashes under some conditions (e.g. with coverage enabled). PR-URL: #29513 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danbev · 2019-09-16T04:01:09Z

Landed in e095e64.

Previously, leaving the exception lying around would leave the JS engine in an invalid state, as it was not expecting exceptions to be thrown from the C++ `PromiseRejectCallback`, and lead to hard crashes under some conditions (e.g. with coverage enabled). PR-URL: #29513 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

gengjiawen · 2020-05-28T08:28:58Z

Just want to say thank you from 2020 😭

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 9, 2019

addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. promises Issues and PRs related to ECMAScript promises. labels Sep 9, 2019

devsnek approved these changes Sep 9, 2019

View reviewed changes

benjamingr approved these changes Sep 10, 2019

View reviewed changes

cjihrig approved these changes Sep 10, 2019

View reviewed changes

jasnell approved these changes Sep 11, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2019

fixup! src: print exceptions from PromiseRejectCallback

b990248

fixup! src: print exceptions from PromiseRejectCallback

b236ffb

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2019

danbev closed this Sep 16, 2019

BridgeAR mentioned this pull request Sep 24, 2019

v12.11.0 proposal #29695

Merged

This was referenced Sep 25, 2019

2019-09-25 Version 12.11.0 (Current) @BridgeAR beevelop/docker-nodejs#301

Closed

2019-09-25 Version 12.11.0 (Current) @BridgeAR beevelop/docker-android-nodejs#333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: print exceptions from PromiseRejectCallback#29513

src: print exceptions from PromiseRejectCallback#29513
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:promise-reject-callback-throw

addaleax commented Sep 9, 2019

Uh oh!

benjamingr commented Sep 9, 2019 •

edited

Loading

Uh oh!

addaleax commented Sep 9, 2019

Uh oh!

benjamingr left a comment

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

Trott commented Sep 12, 2019

Uh oh!

addaleax commented Sep 12, 2019

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

Trott commented Sep 12, 2019

Uh oh!

addaleax commented Sep 13, 2019

Uh oh!

nodejs-github-bot commented Sep 13, 2019

Uh oh!

danbev commented Sep 16, 2019

Uh oh!

gengjiawen commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

addaleax commented Sep 9, 2019

Checklist

Uh oh!

benjamingr commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 9, 2019

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

Trott commented Sep 12, 2019

Uh oh!

addaleax commented Sep 12, 2019

Uh oh!

nodejs-github-bot commented Sep 12, 2019

Uh oh!

Trott commented Sep 12, 2019

Uh oh!

addaleax commented Sep 13, 2019

Uh oh!

nodejs-github-bot commented Sep 13, 2019

Uh oh!

danbev commented Sep 16, 2019

Uh oh!

gengjiawen commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

benjamingr commented Sep 9, 2019 •

edited

Loading